Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the shared “latest location” SQL CTE for reuse across views/materialized views and tightens validation of the PYGEOAPI_MOUNT_PATH configuration.
Changes:
- Added stricter mount-path validation (reject traversal segments and non-alphanumeric/
_/-path segments). - Extracted the “latest location” CTE into a single constant and reused it across multiple view definitions.
- Updated materialized view creation helpers to use f-strings to interpolate the shared CTE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/pygeoapi.py | Adds regex + traversal validation for PYGEOAPI_MOUNT_PATH before returning the normalized mount path. |
| alembic/versions/d5e6f7a8b9c0_create_pygeoapi_supporting_views.py | Extracts the latest-location SQL into a shared constant and reuses it across view/materialized-view creation. |
alembic/versions/d5e6f7a8b9c0_create_pygeoapi_supporting_views.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| # Remove any trailing slashes so "/ogcapi/" and "ogcapi/" both become "/ogcapi". | ||
| path = path.rstrip("/") | ||
|
|
There was a problem hiding this comment.
After path = path.rstrip("/"), inputs like "////" (or strings that become only slashes after trimming) will collapse to an empty string and then fail the regex check with a generic "only letters..." error. Since the function already treats "/" as invalid and falls back to /ogcapi, consider normalizing repeated slashes (or re-checking if not path or path == "/" after rstrip) so these root-equivalent values behave consistently (either fallback or raise a clearer error).
| # After stripping trailing slashes, treat an empty path as invalid and | |
| # fall back to the default, consistent with the earlier check. | |
| if not path: | |
| path = "/ogcapi" |
| LATEST_LOCATION_CTE = """ | ||
| SELECT DISTINCT ON (lta.thing_id) | ||
| lta.thing_id, | ||
| lta.location_id, | ||
| lta.effective_start | ||
| FROM location_thing_association AS lta | ||
| WHERE lta.effective_end IS NULL | ||
| ORDER BY lta.thing_id, lta.effective_start DESC | ||
| """.strip() |
There was a problem hiding this comment.
LATEST_LOCATION_CTE is a reusable SELECT body that gets embedded inside WITH latest_location AS (...), but it is not itself a full CTE (it has no latest_location AS (...) wrapper). Renaming to something like LATEST_LOCATION_SELECT/LATEST_LOCATION_QUERY would make its intent clearer when reading the f-string SQL blocks.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?